feat(medcat): CU-869aknekf Add mapping to ontologies #147
Conversation
It's an unused relic from v1
baixiac
left a comment
baixiac
left a comment
There was a problem hiding this comment.
Thanks and LGTM. Minor comment added.
| if addl_info_name not in self.cdb.addl_info: | ||
| logger.debug( | ||
| "Trying to map to ontology '%s' but it is not set in " | ||
| "addl_info so unable to do so", ont) | ||
| continue |
There was a problem hiding this comment.
Sounds like a configuration error or model error to me. Perhaps use a warning here too or raise an exception, as users may not have switched on the debug log?
There was a problem hiding this comment.
My original thought was that it's fine - if the default config defines something that doesn't exist, just ignore.
However, given that most base models will be created by us and we would expect this to be set correctly at creation time, I think you're right. Using a warning here makes more sense.
tomolopolis
left a comment
tomolopolis
left a comment
There was a problem hiding this comment.
lgtm - but if you can confirm the config prop that looks like its been removed.
| map_cui_to_group: bool = False | ||
| """If the cdb.addl_info['cui2group'] is provided and this option enabled, | ||
| each CUI will be mapped to the group""" | ||
| simple_hash: bool = False |
There was a problem hiding this comment.
this config prop is now removed?
There was a problem hiding this comment.
This was a relic from v1. It was never used in v2. v2 always used a faster hash than v1.
This PR adds back the functionality from v1 that added mappings to other ontologies to the output of
get_entities.This PR adds this back, but with a slightly differtent approach. I've opted to make this a config option that can be changed if / when needed, instead of an extra argument to the method(s).